fix(policies): use structured YAML parsing for policy preset merge by tommylin-signalpro · Pull Request #1055 · NVIDIA/NemoClaw

tommylin-signalpro · 2026-03-28T19:32:10Z

Summary

Replace text-based string manipulation in mergePresetIntoPolicy() with structured YAML parsing via the yaml package
Merge network_policies by name: preset entries override existing on name collision (prevents duplicates on re-apply)
Preserve all non-network sections (filesystem_policy, process, landlock, etc.)
Falls back to text-based approach for non-standard entry formats (backward compatibility)

Problem

The previous implementation used regex and line splitting to inject preset entries into existing policy YAML. This produced invalid YAML when:

The same preset was applied twice (duplicate entries)
Indentation varied between the current policy and preset content
network_policies: appeared at unexpected positions in the document

Changes

bin/lib/policies.js — Rewrite mergePresetIntoPolicy() to parse YAML, merge objects, serialize back. Add yaml as dependency.
test/policies.test.js — Add 3 tests with realistic preset data: structured merge, name collision dedup, non-network section preservation. Relax existing string-format assertions to check correctness instead of exact formatting.

Test plan

All 25 policy tests pass
Full suite: 603/605 pass (2 pre-existing failures in install-preflight.test.js)
Tested with real presets (npm, pypi, slack) on a live sandbox

Summary by CodeRabbit

Bug Fixes
- Improved policy merging to use structured YAML parsing for reliable network policy merges, preserving other top-level sections and ensuring a version field; falls back to previous text-based merge when parsing or merging isn’t possible.
Tests
- Expanded tests for structured merge scenarios, preset overrides/deduplication, and preservation of non-policy sections.
Chores
- Added a YAML parsing library as a runtime dependency.

coderabbitai · 2026-03-28T19:32:24Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1922f0e5-857c-4813-a8dd-d3e9c60c1650

📥 Commits

Reviewing files that changed from the base of the PR and between 3a287af and 2d9dbac.

⛔ Files ignored due to path filters (1)

package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (3)

bin/lib/policies.js
package.json
test/policies.test.js

✅ Files skipped from review due to trivial changes (2)

package.json
test/policies.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

bin/lib/policies.js

📝 Walkthrough

Walkthrough

Replaced fragile line/regex-based policy merging with structured YAML parsing/serialization using the yaml library, merging network_policies maps (preset overrides on key collision), preserving other top-level fields, and falling back to the original text-based merge on parse/format failures.

Changes

Cohort / File(s)	Summary
Core Policy Merge Logic `bin/lib/policies.js`	Reimplemented `mergePresetIntoPolicy` to parse `currentPolicy` and preset entries with `YAML.parse`/`YAML.stringify`, merge `network_policies` maps (preset wins on collisions), preserve non-`network_policies` top-level fields, set `version: current.version
Dependency Addition `package.json`	Added runtime dependency `yaml` (`^2.8.3`) to enable structured parsing/stringifying; bundle list unchanged.
Tests `test/policies.test.js`	Relaxed header assertions to check for `version:` presence and added structured-merge tests verifying map-style preset parsing, proper merging (including override behavior), and preservation of unrelated top-level sections.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibble on keys and hop through the code,
I swapped fragile regex for a structured road.
Maps now merge cleanly, no duplicate fright,
I hop off at dusk, policies tidy and right. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: replacing text-based string manipulation with structured YAML parsing in the policy preset merge logic.
Linked Issues check	✅ Passed	The PR implements all core requirements from `#1010`: structured YAML parsing via yaml library, merging network_policies by key with preset override, preserving non-network sections, fallback for non-standard formats, and valid YAML output generation.
Out of Scope Changes check	✅ Passed	All changes directly support the structured YAML parsing objective: yaml dependency in package.json, refactored mergePresetIntoPolicy with fallback logic, and expanded test coverage for the new merge behavior.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bin/lib/policies.js`:
- Around line 175-176: current.network_policies may be an array, so spreading it
into an object (const mergedNp = { ...existingNp, ...presetPolicies }) will
create numeric keys and corrupt the data; update the merge to guard for arrays:
check Array.isArray(existingNp) (and that presetPolicies is an object) and if
existingNp is an array, do not object-spread — either preserve the array
(mergedNp = existingNp) or convert the array into an object map first (e.g., by
mapping entries by a name/id field) before merging; apply this guard around the
existingNp/mergedNp logic so merges only happen when existingNp is a plain
object.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b7982780-9e9e-45a7-94bb-143e08c56f8b

📥 Commits

Reviewing files that changed from the base of the PR and between eb4ba8c and 2ce593c.

⛔ Files ignored due to path filters (1)

package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (3)

bin/lib/policies.js
package.json
test/policies.test.js

bin/lib/policies.js

wscurran · 2026-03-30T19:05:57Z

✨ Thanks for submitting this fix with a detailed summary, it identifies a bug in the policy preset merge process and proposes a solution using structured YAML parsing, which could improve the stability and reliability of NemoClaw.

kjw3 · 2026-03-30T21:01:27Z

@tommylin-signalpro Thanks for working on this. The core fix direction looks good: replacing the regex/text-based merge with structured YAML parsing is the right approach for #1010, and the targeted policy tests on this branch pass.

I’m not comfortable merging this branch as written for two concrete reasons:

the PR commits need fully verified signatures before it is merge-ready
the branch also removes p-retry from package.json, but current NemoClaw still directly imports p-retry from bin/lib/onboard.js

That second point is an unrelated regression: even if p-retry is still present transitively today, NemoClaw should not rely on a transitive dependency for direct runtime code.

If you update the branch so:

all commits are fully verified/signed
yaml is added without dropping the direct p-retry dependency

then this looks worth re-reviewing.

Fixes NVIDIA#1010 The previous `mergePresetIntoPolicy()` used text-based string manipulation (regex + line splitting) to inject preset entries into the existing policy YAML. This produced invalid YAML when: - Preset entries were re-applied (duplicates) - Indentation varied between current policy and preset - network_policies appeared at unexpected positions Replace with structured YAML merge using the `yaml` package: - Parse both current policy and preset entries as YAML objects - Merge network_policies by name (preset overrides on collision) - Preserve all non-network sections (filesystem_policy, process, etc.) - Ensure version header exists Falls back to the text-based approach when preset entries use non-standard list format (backward compatibility with existing callers). Added 3 new tests: - Structured merge with realistic preset data - Deduplication on policy name collision - Preservation of non-network sections during merge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Address CodeRabbit review: existing network_policies may be an array in legacy policies. Spreading an array into an object produces numeric keys ("0", "1") and corrupts the data. Now checks Array.isArray() before merging — falls back to using preset entries only when existing is not a plain object. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…lexity Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

tommylin-signalpro · 2026-03-31T01:50:03Z

@wscurran Thanks for the kind words!

@kjw3 Thanks for the review. Both issues are addressed:

Commit signatures: Rebased the branch onto main with SSH-signed commits (both are now signed with a verified key).
p-retry dependency: Restored p-retry and bundleDependencies in package.json — yaml is now added alongside it, not replacing it.

Ready for re-review.

coderabbitai bot reviewed Mar 28, 2026

View reviewed changes

bin/lib/policies.js Outdated Show resolved Hide resolved

wscurran added bug Something isn't working fix labels Mar 30, 2026

kjw3 self-assigned this Mar 30, 2026

tommylin-signalpro and others added 3 commits March 31, 2026 09:44

refactor: extract textBasedMerge to reduce mergePresetIntoPolicy comp…

2d9dbac

…lexity Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

tommylin-signalpro force-pushed the fix/structured-yaml-policy-merge branch from 3a287af to 2d9dbac Compare March 31, 2026 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(policies): use structured YAML parsing for policy preset merge#1055

fix(policies): use structured YAML parsing for policy preset merge#1055
tommylin-signalpro wants to merge 3 commits intoNVIDIA:mainfrom
tommylin-signalpro:fix/structured-yaml-policy-merge

tommylin-signalpro commented Mar 28, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 28, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

wscurran commented Mar 30, 2026

Uh oh!

kjw3 commented Mar 30, 2026

Uh oh!

tommylin-signalpro commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tommylin-signalpro commented Mar 28, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wscurran commented Mar 30, 2026

Uh oh!

kjw3 commented Mar 30, 2026

Uh oh!

tommylin-signalpro commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tommylin-signalpro commented Mar 28, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 28, 2026 •

edited

Loading